add: [Windows] Drive root parse fix #3328

djdv · 2016-10-25T17:25:01Z

Previously when adding the root of a drive on Windows (either directly through X:\ or via . while in X:), ipfs would prefix everything and not handle directories properly in a manner simillar to this issue:#1922

This does not happen with paths or files underneath the root, it's just when adding the root itself. 👻
This can be fixed by preserving the full path X:\ instead of using X: as a base when specifying the root of a drive only. As an aside it would not be valid to use / instead, since / is relative to the drive in your working directory on Windows, not an absolute path. Using / while adding Y:\ from X:\ would either give you N contents from X: or panic if X: contains less items in the root than Y: does. Preserving the full path doesn't have this issue.

I make sure not to act on paths starting with / and check for a backslash, to my knowledge a valid input path like that should only show up on Windows.
Using long path prefix form \\?\x: will still give the incorrect behavior, I don't think it's necessary to check for this but it could be caught in the same way, then converted from the long form to the short form X:\

Any issues with this and should long form be handled too?

whyrusleeping · 2016-10-28T04:32:16Z

Can some other windows user verify and test this? @slothbag ?

djdv · 2016-10-28T20:09:40Z

Changed comment, "drive letter" -> "drive path".

Kubuxu · 2016-10-28T20:24:10Z

@mateon1 could you give it a test.

mateon1 · 2016-11-03T02:52:12Z

Sorry for the late response. I gave this PR a test. It doesn't seem to fix the problem, the ipfs add output is the same for both this PR and the parent commit.
Here are my results
master.txt is 68d8a29 and pr.txt is f2b271c

djdv · 2016-11-03T10:53:51Z

Thanks @mateon1
X: isn't handled by this yet, only X:\ or X:/. When using . in X:\ it should have worked as expected though, I'm not actually sure why that didn't catch since . should resolve to X:\ with os.Getwd(), the same test works on my actual system and I just tried it inside a vm with a fresh Windows install. Both give these results (note the long form still giving bad results since it's unchecked for now).

I also rebuilt with Go 1.7.3 and still get the same. I'm not sure where the discrepancy could be, any idea?

I'm also not sure how X: on its own should be handled, it could be translated to X:\ but I think it may be best to return an invalid path error, with a message explaining to add a \ to the input if you intend to add an entire root. The only use of X: by itself that I know of is to change the active drive in the command interpreter, so it shouldn't be a valid path target on its own. X:file.ext(no slash but has file or path) however is a valid target, it points to the file relative to the current working directory (not necessarily the root) of drive X: which will add correctly even recursively.

I'd need some guidance on what the behavior should be for both X: and the long form \\?\x:, convert them, return an error, or leave as is (bad behavior).

Sorry for the verbosity but I have to be pedantic with these DOS path semantics and behaviors.

djdv · 2016-11-12T19:31:27Z

Alright, so I wrote something here that should properly handle all cases and hopefully not break anything.
X: and \\?\X: will return an error asking you to append a \ to your input.
X:\ and \\?\X:\ should work as expected
X:. and X:\.\somepath should also work as expected, the relative paths being resolved into absolute paths and then handled like normal.
. while in a drive root should also work as expected.

If anyone has suggestions for improvement, I'd like to hear them. I'm also not sure if the comments are overkill, but these are weird Windows specific special cases.

Everything works as normal in my case but I'd like others to test this if they can. If everything looks good I'll squash these and rebase on master.

Pinging @slothbag, @mateon1, and any other Windows users that see this.

Obligatory shell shot.

slothbag · 2016-11-14T09:44:58Z

Unfortunately I don't have the time to test this right now, but looking at the screenshot that pretty much covers the scenarios I would run.. It looks pretty good to me.

whyrusleeping

Lets make sure to consider the effects these changes will have on linux and OSX systems. (among others)

whyrusleeping · 2016-11-25T18:56:45Z

commands/cli/parse.go

+	switch len(fpath) {
+	case 3:
+		// X: will be cleaned to `X:.` which is not what is intended, a case for handling dot relative paths is above
+		if fpath[0] != '/' && fpath[1:3] == ":." {


Hrm, we need to special case this to only run on windows. In linux, X:. is a perfectly valid filename

djdv · 2016-11-26T00:27:57Z

@whyrusleeping
You think it would be best to check the OS at runtime for this? I was trying to figure out a way to avoid it by using Go build constraints so that special case checking was only built in on platforms that need it, but I'm not super familiar with them for this kind of thing.

Kubuxu · 2016-11-26T00:57:27Z

See https://github.com/ipfs/go-ipfs/blob/9094cc8f4297b9365a3d3260f839724b135094d0/cmd/ipfs/main.go#L607-L613

djdv · 2016-12-06T01:17:16Z

Modified the comments to have consistent quotations and changed some wording, also shortened the error message. Unless anyone has any issues with the logic or the wording, this should be good to go.

djdv · 2017-03-06T18:30:46Z

@whyrusleeping o/
Any chance I could have this looked at? I was hoping to have this merged before any filestore changes, I feel like adding entire drives+no-copy will go hand in hand.

whyrusleeping

One request, then you'll need to rebase this to fix the merge conflicts.

After that this LGTM

whyrusleeping · 2017-03-06T18:39:33Z

commands/cli/parse.go

+	// special cases for Windows drive roots i.e. `X:\` and their long form `\\?\X:\`
+	// drive path must be preserved as `X:\` (or it's longform) and not converted to `X:`, `X:.`, `\`, or `/` here
+	if osh.IsWindows() {
+		switch len(fpath) {


can we extract this out into a windowsParseFile sort of thing?

djdv · 2017-03-06T19:42:42Z

Like this?

License: MIT Signed-off-by: Dominic Della Valle <ddvpublic@gmail.com>

whyrusleeping · 2017-03-08T01:12:20Z

@djdv Yeah, though now tests are failing strangely... It looks like an issue that we've already fixed.

cc @Kubuxu, could you take a look?

whyrusleeping · 2017-03-10T22:46:01Z

Going to assume the travis failure is an issue with caching old code.

djdv changed the title ~~Windows root parse fix~~ add: [Windows] Drive root parse fix Oct 25, 2016

whyrusleeping added the need/review Needs a review label Oct 28, 2016

djdv force-pushed the win-driveroot-fix branch from 10b5f22 to f2b271c Compare October 28, 2016 20:07

djdv force-pushed the win-driveroot-fix branch 3 times, most recently from d431c12 to b59dcb5 Compare November 12, 2016 19:06

whyrusleeping reviewed Nov 25, 2016

View reviewed changes

djdv force-pushed the win-driveroot-fix branch from b59dcb5 to 7f92b0d Compare November 26, 2016 01:49

whyrusleeping added the status/ready Ready to be worked label Nov 28, 2016

djdv force-pushed the win-driveroot-fix branch 2 times, most recently from ee06194 to 0aa5a88 Compare December 6, 2016 01:07

djdv force-pushed the win-driveroot-fix branch from 0aa5a88 to 79b285a Compare February 12, 2017 20:40

whyrusleeping requested changes Mar 6, 2017

View reviewed changes

djdv force-pushed the win-driveroot-fix branch 3 times, most recently from 5a1d3b7 to eed8528 Compare March 6, 2017 19:36

djdv added 2 commits March 7, 2017 09:10

Replace OS check

934a073

License: MIT Signed-off-by: Dominic Della Valle <ddvpublic@gmail.com>

Windows root parse fix

b29945e

License: MIT Signed-off-by: Dominic Della Valle <ddvpublic@gmail.com>

djdv force-pushed the win-driveroot-fix branch from eed8528 to b29945e Compare March 7, 2017 14:11

whyrusleeping added this to the Ipfs 0.4.7 milestone Mar 8, 2017

whyrusleeping merged commit eecc766 into ipfs:master Mar 10, 2017

whyrusleeping removed the status/ready Ready to be worked label Mar 10, 2017

djdv mentioned this pull request Mar 12, 2018

Windows initiative 2018 #4808

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: [Windows] Drive root parse fix #3328

add: [Windows] Drive root parse fix #3328

djdv commented Oct 25, 2016

whyrusleeping commented Oct 28, 2016

djdv commented Oct 28, 2016

Kubuxu commented Oct 28, 2016

mateon1 commented Nov 3, 2016

djdv commented Nov 3, 2016

djdv commented Nov 12, 2016

slothbag commented Nov 14, 2016 •

edited

Loading

whyrusleeping left a comment

whyrusleeping Nov 25, 2016

djdv commented Nov 26, 2016

Kubuxu commented Nov 26, 2016

djdv commented Dec 6, 2016

djdv commented Mar 6, 2017

whyrusleeping left a comment

whyrusleeping Mar 6, 2017

djdv commented Mar 6, 2017

whyrusleeping commented Mar 8, 2017

whyrusleeping commented Mar 10, 2017

add: [Windows] Drive root parse fix #3328

add: [Windows] Drive root parse fix #3328

Conversation

djdv commented Oct 25, 2016

whyrusleeping commented Oct 28, 2016

djdv commented Oct 28, 2016

Kubuxu commented Oct 28, 2016

mateon1 commented Nov 3, 2016

djdv commented Nov 3, 2016

djdv commented Nov 12, 2016

slothbag commented Nov 14, 2016 • edited Loading

whyrusleeping left a comment

Choose a reason for hiding this comment

whyrusleeping Nov 25, 2016

Choose a reason for hiding this comment

djdv commented Nov 26, 2016

Kubuxu commented Nov 26, 2016

djdv commented Dec 6, 2016

djdv commented Mar 6, 2017

whyrusleeping left a comment

Choose a reason for hiding this comment

whyrusleeping Mar 6, 2017

Choose a reason for hiding this comment

djdv commented Mar 6, 2017

whyrusleeping commented Mar 8, 2017

whyrusleeping commented Mar 10, 2017

slothbag commented Nov 14, 2016 •

edited

Loading